Added initial commit of camunda instrumentation #12830
Conversation
|
|
|
@laurit can you please review this |
There are no tests. You didn't add your module to |
|
Thanks @cb645j for starting this submission. Yes @laurit is correct, this needs to be integrated with settings.gradle and the full test suite. A few questions. There a lot of files in this PR, I counted 7 distinct TypeInstrumentations for the following Camunda classes:
First question: Can you merge the instrumentation for the like-components?... I.e. All the "Behavior" components go through a single TypeInstrumentation? Second question: What about the version, does this handle only Camunda 7? or does this handle camunda 7 and greater? What version is targeted? Thanks |
Thanks Steve. I think I can probably merge a few of those typeinstrumentations into single classes, i will take a look! Yes, i tested with versions 7 and 8 but I believe it should handle anything above 7 as I did not see anything in later versions that would break so i left the max version undefined in the settings muzzle.pass, I think thats what your suppose to do when it supports version x and above? Camunda noticed that will no longer support versions 6 and under so I did not worry about those ones. |
Opps, I add it as a module but forgot to check that file in, ill update.. Ill work on adding unit test. thanks |
@cb645j Cory, that v7 and above support is great. Recommendation 1: versioning: I'd recommend moving everything into directory that states "camunda-7", which covers all of 7 and possibly 8 if the right classes. This version 7, 8 support can be is enforced in the override of the classLoaderMatcher() method of InstrumentationModule, you want to choose a class that is really unique to version 7 at least. Also, you muzzle config will want to drop back down to the 7.0 or the lowest level of version 7 you aim to support. Recommendation 2: folder structure: I'd recommend restructurctuing the entire thing to adhere to the Folder Structure Style Guide. where you have 3 main directories, javaagent, library and testing directories. Recommendation 3: testing: |
This only makes sense if you are going to have both library and agent instrumentations. If you are going to have only the javaagent instrumentation only that module is enough. No need to have a separate testing module.
Again if you are going to have only the agent instrumentation then you don't really need to create abstract test classes that have only 1 implementaion. |
|
Thanks @laurit for clearing up the Library vs Agent instrumentation. I was assuming that the rule was you need to create both as that was what I was required to do for Jetty-Httpclient-9.2. So, now, if we don't feel the necessity for a Library-based instrumentation, then we can choose to simply implement Javagent only? Thanks |
I took another look at the Writing Instumentation doc. It seems the rule of thumb is if you have target classes that can support Library-style instrumentation, then you should provide the library feature as an option. |
If a library instrumentation can be built we definitely would like to have one, but it is not always possible to write one. |
|
Thank you for working on this! This will be a great help to many of my projects as we use SpringBoot and Camunda 7 extensively. Recently we have started integrating with Datadog. Request: can you include the businessKey in the CamundaCommonRequest and code that populates it? That would help us identify the traces/spans. I am new to OpenTelemetry, Observability and Instrumentation in general. I have quite a lot of experience with Java, Spring and Camunda. If there is something I could to do help with this, please let me know! |
Yes, I should be able to add the businesskey. As far as implementation specific variables, i did plan to add something exactly as your describing - the ability for the user to identify additional execution variables they want to be captured (via a comma separated config property), however I plan to make this change as a separate enhancement after I get this PR merged. I would like to just get this basic implementation in before making additional enhancements, but i can add the businesskey as part of this PR since that is a global camunda property and will be easy to do. |
|
@laurit , all checks have passed. can you please review |
there are still no tests, do you plan to add them? |
@laurit I looked into it, and there is really no way to add unit test, at least not any that add any king of value. I did conduct thorough e2e test though with multiple applications and verified it works and works as expected. Can you please approve. |
|
hi @cb645j, I don't think we'd accept instrumentation in this repository without tests. You could always host it yourself and release it as an OpenTelemetry Java agent extension for people to use. |
|
@laurit @trask @jeanbisutti i have added test, can you please review. Thank you!! |
| @@ -0,0 +1,16 @@ | |||
| plugins { | |||
There was a problem hiding this comment.
By library instrumentation we mean an instrumentation that can be appleid manually by including the instrumentation jar in your application and doing some code changes if needed. For an example have a look at https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/okhttp/okhttp-3.0/library/README.md What you have here doesn't look like a library instrumentation, but a collection of classes that are used in the agent instrumentation. If you don't wish to build a library instrumentation I'd suggest you merge the library and testing modules into the javaagent module.
| span -> | ||
| span.hasName("Get Product Info Task") | ||
| .hasKind(SpanKind.INTERNAL) | ||
| .hasAttributesSatisfyingExactly( |
There was a problem hiding this comment.
usually we also assert that the span has the expected parent with .hasNoParent() or .hasParent(trace.getSpan(0))
| if (Java8BytecodeBridge.currentContext() == Java8BytecodeBridge.rootContext()) { | ||
| // log | ||
| } |
| @Override | ||
| public boolean defaultEnabled(ConfigProperties config) { | ||
| return config.getBoolean("otel.instrumentation.common.default-enabled", true); | ||
| } |
| String[] helperClassnames = { | ||
| "io.opentelemetry.javaagent.instrumentation.camunda.v7_0.behavior", | ||
| "io.opentelemetry.instrumentation.camunda.v7_0.behavior", | ||
| "io.opentelemetry.instrumentation.camunda.v7_0.common" | ||
| }; | ||
|
|
||
| @Override | ||
| public boolean isHelperClass(String classname) { | ||
| return super.isHelperClass(classname) | ||
| || Arrays.stream(helperClassnames).anyMatch(c -> classname.startsWith(c)); | ||
| } |
| @Override | ||
| public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() { | ||
| return hasClassesNamed( | ||
| "org.camunda.bpm.engine.impl.bpmn.behavior.TaskActivityBehavior", | ||
| "org.camunda.bpm.engine.impl.bpmn.behavior.NoneEndEventActivityBehavior", | ||
| "org.camunda.bpm.engine.impl.bpmn.behavior.ErrorEndEventActivityBehavior"); | ||
| } |
There was a problem hiding this comment.
Usually you don't need this. Matchers apply as classes are loaded, no need to check whether the classes you intend to instrument are present. Typically this method is used when instrumentation needs some class that is present only starting from some version.
| module.set("camunda-engine") | ||
|
|
||
| // have not tested with versions prior to 7.18.0 | ||
| versions.set("[7.18.0,)") |
There was a problem hiding this comment.
Usually we also add assertInverse.set(true). This verifies that the instrumentation does not apply to versions outside the given range. If the instrumentation applies to earlier versions you have the choice of either expanding the supported version range or add a classLoaderMatcher method that checks for a class that was added in the first version you intend to support (this will make the instrumentation not apply to earlier versions). If the first supported version is 7.18 then the module should be named accordingly.
| import net.bytebuddy.matcher.ElementMatcher; | ||
|
|
||
| @AutoService(InstrumentationModule.class) | ||
| public class CamundaCommonBehaviorModule extends InstrumentationModule { |
There was a problem hiding this comment.
consider whether you really need separate instrumentation modules or whether you can merge them (the same applies for the Singletons classes)
| @Override | ||
| public ElementMatcher<ClassLoader> classLoaderOptimization() { | ||
| return hasClassesNamed("org.camunda.bpm.engine.impl.jobexecutor.AsyncContinuationJobHandler"); | ||
| } |
There was a problem hiding this comment.
this method is not needed when matching is done only based on the class name. This method is usually used by instrumentations that match based on class hierarchy.
related to issue #12122